Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add control for removing stopped docker containers. #1290

Merged
merged 5 commits into from
Apr 22, 2016

Conversation

tomwilkie
Copy link
Contributor

@tomwilkie tomwilkie commented Apr 13, 2016

Also fixes #1367

@paulbellamy
Copy link
Contributor

Maybe after the play button? Or is there a specific reason for putting it first?

@paulbellamy
Copy link
Contributor

Also, removing a container takes 15 seconds (so looks like the control hasn't worked), then the details panel breaks (but stays open).

@tomwilkie
Copy link
Contributor Author

Maybe after the play button? Or is there a specific reason for putting it first?

No reason, will do.

Also, removing a container takes 15 seconds (so looks like the control hasn't worked), then the details panel breaks (but stays open).

I have no intention of dealing with this in this PR, if thats alright.

@tomwilkie
Copy link
Contributor Author

Maybe after the play button? Or is there a specific reason for putting it first?

No reason, will do.

Actually, there sorted alphabetically. We can't control the order of them.

@tomwilkie
Copy link
Contributor Author

Also, removing a container takes 15 seconds (so looks like the control hasn't worked),

This will be fixed in #1363

then the details panel breaks (but stays open).

@davkal / @foot any ideas here? We leave the details panel open for good reason (reconnects etc), but perhaps you could close it if you receive a Remove node diff? But not if the ws goes away?

@foot
Copy link
Contributor

foot commented Apr 21, 2016

The details panel can handle 404s initially. Perhaps if it gets a 404 while waiting for a control to complete something bad happens. Will have a look.

@davkal
Copy link
Contributor

davkal commented Apr 21, 2016

Yes, we can close the details panel on node remove (we're already handling missing mouseout events that way). Otherwise we could do control update requests to get the status on a control action.

@davkal davkal assigned davkal and unassigned davkal Apr 21, 2016
@davkal
Copy link
Contributor

davkal commented Apr 21, 2016

I propose this lifecycle-ish order: start - restart - pause - stop - remove

@davkal
Copy link
Contributor

davkal commented Apr 21, 2016

Could also be done in-band, in the control response. We just need to agree on a field. Eg. terminal controls return {pipe}, a remove control could return a different field. @tomwilkie

@tomwilkie
Copy link
Contributor Author

tomwilkie commented Apr 21, 2016

Sure, I can return a field saying along the lines of "deleteNode": "<nodeid>"?

Scratch that, lets be explicit. "closeDetails": true

@tomwilkie tomwilkie force-pushed the remove-container-control branch from e35c982 to ed555b6 Compare April 21, 2016 15:41
@davkal davkal assigned tomwilkie and unassigned davkal Apr 21, 2016
@tomwilkie
Copy link
Contributor Author

I think thats all the feedback addressed - @paulbellamy PTAL?

@tomwilkie tomwilkie assigned paulbellamy and unassigned tomwilkie Apr 21, 2016
RawTTY bool `json:"raw_tty,omitempty"`

// Remove specific fields
RemovedNode string `json:"removedNode,omitempty"` // Set if node was removed

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

@paulbellamy paulbellamy assigned tomwilkie and unassigned paulbellamy Apr 22, 2016
@davkal davkal assigned davkal and unassigned tomwilkie Apr 22, 2016
@davkal
Copy link
Contributor

davkal commented Apr 22, 2016

I'm treating the removal in the UI only. closeOnSuccess is the UI's domain, the backend should not know about it.

@davkal davkal assigned paulbellamy and unassigned davkal Apr 22, 2016
@davkal davkal force-pushed the remove-container-control branch from 50df0d4 to eda2a20 Compare April 22, 2016 10:44
@paulbellamy paulbellamy merged commit 1539e66 into master Apr 22, 2016
@paulbellamy paulbellamy deleted the remove-container-control branch April 22, 2016 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

We should be able to control the order of controls in the details panel
4 participants